Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a standalone client for TriliumNext, enabling it to run in a browser environment using a SQLite WASM backend. It includes the necessary infrastructure for service workers, a browser-compatible router, and platform-specific providers. My review highlights critical security concerns regarding hardcoded secrets, high-severity issues with service worker configuration and synchronous crypto API usage, and potential CORS misconfigurations. I have also suggested refactoring the worker initialization logic for better maintainability and improving the reliability of device detection.
| const STATIC_CACHE = `static-${VERSION}`; | ||
|
|
||
| // Check if running in dev mode (passed via URL parameter) | ||
| const isDev = true; |
There was a problem hiding this comment.
The isDev constant is hardcoded to true, which will disable service worker caching in all environments, including production. This should be set dynamically based on the build environment. Vite can replace import.meta.env.DEV with true or false at build time, which would be a more robust solution.
| const isDev = true; | |
| const isDev = import.meta.env.DEV; |
| if (config["Network"]["corsAllowOrigin"]) { | ||
| res.header("Access-Control-Allow-Origin", config["Network"]["corsAllowOrigin"]); | ||
| res.header("Access-Control-Allow-Credentials", "true"); | ||
| } |
There was a problem hiding this comment.
Setting Access-Control-Allow-Credentials to true is only valid when Access-Control-Allow-Origin is set to a specific origin, not a wildcard (*). If your configuration allows for a wildcard origin, this will cause CORS errors in the browser. Please add a check to ensure Access-Control-Allow-Credentials is only set when the origin is not a wildcard.
| if (config["Network"]["corsAllowOrigin"]) { | |
| res.header("Access-Control-Allow-Origin", config["Network"]["corsAllowOrigin"]); | |
| res.header("Access-Control-Allow-Credentials", "true"); | |
| } | |
| const origin = config["Network"]["corsAllowOrigin"]; | |
| if (origin) { | |
| res.header("Access-Control-Allow-Origin", origin); | |
| if (origin !== '*') { | |
| res.header("Access-Control-Allow-Credentials", "true"); | |
| } | |
| } |
| async function initialize(): Promise<void> { | ||
| if (initPromise) { | ||
| return initPromise; // Already initializing | ||
| } | ||
| if (initError) { | ||
| throw initError; // Failed before, don't retry | ||
| } | ||
|
|
||
| initPromise = (async () => { | ||
| try { | ||
| // First, load all modules dynamically | ||
| await loadModules(); | ||
|
|
||
| console.log("[Worker] Initializing SQLite WASM..."); | ||
| await sqlProvider!.initWasm(); | ||
|
|
||
| // Try to use OPFS for persistent storage | ||
| if (sqlProvider!.isOpfsAvailable()) { | ||
| console.log("[Worker] OPFS available, loading persistent database..."); | ||
| sqlProvider!.loadFromOpfs("/trilium.db"); | ||
| } else { | ||
| // Fall back to in-memory database (non-persistent) | ||
| console.warn("[Worker] OPFS not available, using in-memory database (data will not persist)"); | ||
| console.warn("[Worker] To enable persistence, ensure COOP/COEP headers are set by the server"); | ||
| sqlProvider!.loadFromMemory(); | ||
| } | ||
|
|
||
| console.log("[Worker] Database loaded"); | ||
|
|
||
| console.log("[Worker] Loading @triliumnext/core..."); | ||
| const schemaModule = await import("@triliumnext/core/src/assets/schema.sql?raw"); | ||
| coreModule = await import("@triliumnext/core"); | ||
| await coreModule.initializeCore({ | ||
| executionContext: new BrowserExecutionContext(), | ||
| crypto: new BrowserCryptoProvider(), | ||
| messaging: messagingProvider!, | ||
| request: new FetchRequestProvider(), | ||
| platform: new StandalonePlatformProvider(), | ||
| translations: translationProvider, | ||
| schema: schemaModule.default, | ||
| dbConfig: { | ||
| provider: sqlProvider!, | ||
| isReadOnly: false, | ||
| onTransactionCommit: () => { | ||
| coreModule?.ws.sendTransactionEntityChangesToAllClients(); | ||
| }, | ||
| onTransactionRollback: () => { | ||
| // No-op for now | ||
| } | ||
| } | ||
| }); | ||
| coreModule.ws.init(); | ||
|
|
||
| console.log("[Worker] Supported routes", Object.keys(coreModule.routes)); | ||
|
|
||
| // Create and configure the router | ||
| router = createConfiguredRouter(); | ||
| console.log("[Worker] Router configured"); | ||
|
|
||
| // initializeDb runs initDbConnection inside an execution context, | ||
| // which resolves dbReady — required before beccaLoaded can settle. | ||
| coreModule.sql_init.initializeDb(); | ||
|
|
||
| if (coreModule.sql_init.isDbInitialized()) { | ||
| console.log("[Worker] Database already initialized, loading becca..."); | ||
| await coreModule.becca_loader.beccaLoaded; | ||
| } else { | ||
| console.log("[Worker] Database not initialized, skipping becca load (will be loaded during DB initialization)"); | ||
| } | ||
|
|
||
| console.log("[Worker] Initialization complete"); | ||
| } catch (error) { | ||
| initError = error instanceof Error ? error : new Error(String(error)); | ||
| console.error("[Worker] Initialization failed:", initError); | ||
| throw initError; | ||
| } | ||
| })(); | ||
|
|
||
| return initPromise; | ||
| } |
There was a problem hiding this comment.
The initialize function is quite long and handles multiple distinct steps (loading modules, initializing SQLite, loading the database, initializing the core). For better readability and maintainability, consider refactoring this into smaller, more focused helper functions, each responsible for a single part of the initialization process. For example:
private async _loadModules() { /* ... */ }
private async _initializeDatabase() { /* ... */ }
private async _initializeCore() { /* ... */ }
async function initialize() {
// ...
await this._loadModules();
await this._initializeDatabase();
await this._initializeCore();
// ...
}| function isMobile() { | ||
| const mQ = matchMedia?.("(pointer:coarse)"); | ||
| if (mQ?.media === "(pointer:coarse)") return !!mQ.matches; | ||
|
|
||
| if ("orientation" in window) return true; | ||
| const userAgentsRegEx = /\b(Android|iPhone|iPad|iPod|Windows Phone|BlackBerry|webOS|IEMobile)\b/i; | ||
| return userAgentsRegEx.test(navigator.userAgent); | ||
| } |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| // TODO: Remove after porting the file | ||
| // @ts-ignore | ||
| const bundleService = (await import("./bundle.js")).default as any; | ||
| // TODO: Remove after porting the file | ||
| // @ts-ignore | ||
| const froca = (await import("./froca.js")).default as any; |
There was a problem hiding this comment.
The use of @ts-ignore with dynamic import() suggests a potential circular dependency or a need for lazy loading. While this works, it bypasses TypeScript's type safety. Consider refactoring to break the cycle, perhaps by passing dependencies into this function or using a more structured dependency injection approach. This would make the code safer and easier to reason about.
This reverts commit c7cf8d5.
|
🖥️ App preview is ready! 🔗 Preview URL: https://pr-9189.trilium-app.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
| } else if (!val.includes("`")) { | ||
| return `\`${val}\``; | ||
| } else { | ||
| return `"${val.replace(/"/g, '\\"')}"`; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 28 days ago
In general, when implementing escaping manually, ensure that all meta-characters that are significant in the target syntax are properly escaped. When using backslash escapes inside double quotes, this includes both the quote character itself and the backslash. Failing to escape backslashes can allow sequences like \" or \n to be interpreted in unintended ways.
For this specific code, the final else in formatValue is the “complex case” used when the value contains all three quote characters. In that branch, the code wraps the string in double quotes and replaces only " with \". To make escaping consistent and robust, we should first escape backslashes (\ → \\), and then escape double quotes (" → \") on this already backslash-escaped string. This avoids any interference between escapes and ensures every backslash in the original value becomes \\ in the quoted result. Concretely, on line 40 we should change the return expression so that it applies two chained replace calls: one for backslashes with a global regex /\\/g, and one for double quotes with /"/g. No new imports are needed, and no other parts of the file must change.
| @@ -37,7 +37,7 @@ | ||
| } else if (!val.includes("`")) { | ||
| return `\`${val}\``; | ||
| } else { | ||
| return `"${val.replace(/"/g, '\\"')}"`; | ||
| return `"${val.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; | ||
| } | ||
| } | ||
|
|
| const imageRe = /<img[^>]*?\ssrc=['"]([^'">]+)['"]/gi; | ||
| let imageMatch; | ||
|
|
||
| while ((imageMatch = imageRe.exec(content))) { |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
| content = `${content.substring(0, imageMatch.index)}<img src="api/attachments/${attachment.attachmentId}/image/${encodedTitle}"${content.substring(imageMatch.index + imageMatch[0].length)}`; | ||
| } else if ( | ||
| !url.includes("api/images/") && | ||
| !/api\/attachments\/.+\/image\/?.*/.test(url) && |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
| const inlineAttachmentRe = /<a[^>]*?\shref=['"]data:([^;'">]+);base64,([^'">]+)['"][^>]*>(.*?)<\/a>/gim; | ||
| let attachmentMatch; | ||
|
|
||
| while ((attachmentMatch = inlineAttachmentRe.exec(content))) { |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
| } | ||
|
|
||
| export function stripTags(text: string) { | ||
| return text.replace(/<(?:.|\n)*?>/gm, ""); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
Use a robust, iterative removal strategy so tag patterns cannot reappear after a single pass. The best minimal fix (without adding dependencies or changing existing behavior significantly) is to repeatedly apply the same regex until the string no longer changes. This directly addresses the “incomplete multi-character sanitization” class and keeps the function contract (return plain text with tags removed).
In packages/trilium-core/src/services/utils/index.ts, update stripTags (around lines 377–379) to:
- keep the same regex semantics for compatibility,
- run replacement in a loop (
do...while) until a fixed point is reached, - return the fully stabilized result.
No new imports or external packages are required.
| @@ -375,7 +375,14 @@ | ||
| } | ||
|
|
||
| export function stripTags(text: string) { | ||
| return text.replace(/<(?:.|\n)*?>/gm, ""); | ||
| let previous: string; | ||
|
|
||
| do { | ||
| previous = text; | ||
| text = text.replace(/<(?:.|\n)*?>/gm, ""); | ||
| } while (text !== previous); | ||
|
|
||
| return text; | ||
| } | ||
|
|
||
| export function toObject<T, K extends string | number | symbol, V>(array: T[], fn: (item: T) => [K, V]): Record<K, V> { |
| if (protect !== note.isProtected) { | ||
| const content = note.getContent(); | ||
|
|
||
| note.isProtected = protect; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
Use a safe note lookup API (becca.getNoteOrThrow(noteId)) instead of raw dictionary indexing (becca.notes[noteId]) in the route handler. This preserves functionality (valid note IDs still work exactly the same) while preventing special prototype keys from being interpreted as object meta-properties.
Best targeted fix:
- File:
packages/trilium-core/src/routes/api/notes.ts - Region:
protectNotehandler (around current lines 218–221) - Replace:
const note = becca.notes[noteId];
- With:
const note = becca.getNoteOrThrow(noteId);
No new imports, helper methods, or dependencies are required.
| @@ -217,7 +217,7 @@ | ||
|
|
||
| function protectNote(req: Request<{ noteId: string; isProtected: string }>) { | ||
| const noteId = req.params.noteId; | ||
| const note = becca.notes[noteId]; | ||
| const note = becca.getNoteOrThrow(noteId); | ||
| const protect = !!parseInt(req.params.isProtected); | ||
| const includingSubTree = !!parseInt(req.query?.subtree as string); | ||
|
|
Patch DOMParser.parseFromString in the standalone vitest setup to strip the leading LF after <pre>/<listing>/<textarea>, matching the HTML spec behavior that turnish relies on in Node (domino). Decode attachment content via decodeUtf8 in the ENEX spec so binary bytes don't get comma-stringified as a Uint8Array.
…nto standalone
| name: Build Android APK | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout the repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - uses: pnpm/action-setup@v6 | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: 24 | ||
| cache: "pnpm" | ||
|
|
||
| - name: Set up JDK 21 | ||
| uses: actions/setup-java@v5 | ||
| with: | ||
| distribution: temurin | ||
| java-version: 21 | ||
|
|
||
| - name: Set up Gradle | ||
| uses: gradle/actions/setup-gradle@v5 | ||
|
|
||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Update build info | ||
| run: pnpm run chore:update-build-info | ||
|
|
||
| - name: Build client-standalone (webDir for Capacitor) | ||
| run: pnpm --filter @triliumnext/mobile build | ||
|
|
||
| - name: Sync Capacitor Android project | ||
| run: pnpm --filter @triliumnext/mobile exec cap sync android | ||
|
|
||
| - name: Assemble debug APK | ||
| working-directory: apps/mobile/android | ||
| run: ./gradlew assembleDebug --no-daemon | ||
|
|
||
| - name: Upload APK | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: trilium-mobile-debug-apk | ||
| path: apps/mobile/android/app/build/outputs/apk/debug/*.apk | ||
| retention-days: 14 |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
Add an explicit permissions block in .github/workflows/mobile.yml at the workflow root (top-level), so it applies to all jobs unless overridden.
For this workflow, the minimal safe baseline is:
contents: read
This preserves existing behavior (checkout/build/upload-artifact) while enforcing least privilege for GITHUB_TOKEN. No imports, methods, or dependencies are needed since this is YAML configuration only.
| @@ -4,6 +4,9 @@ | ||
| push: | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
pnpm-lock.yamlfrom mergingorigin/mainintostandalone